Conversation
- Fix path traversal in TypeScript YAML parser: use proper path segment check instead of substring, ensure consistent validation/access paths - Add missing getattr to Python sandbox blocking to prevent attribute access bypasses - Enhance command sanitization: add missing pipe, semicolon, newline patterns - Improve path validation: use robust Path.relative_to() instead of string prefix matching - Fix type hints and remove unused imports in agent tools Addresses security issues identified by code reviewers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR hardens path traversal and command injection validation across three codebases: it replaces string-based path prefix checks with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoFix critical path traversal and sandbox escape vulnerabilities
WalkthroughsDescription• Fix critical path traversal vulnerabilities using robust Path.relative_to() method • Add missing getattr to Python sandbox blocking to prevent attribute access bypasses • Enhance command sanitization with pipe, semicolon, newline patterns • Improve TypeScript YAML parser path validation with proper segment checking • Fix type hints and imports in agent tools module Diagramflowchart LR
A["Path Validation Issues"] -->|Replace string prefix| B["Use Path.relative_to()"]
C["Python Sandbox Gaps"] -->|Add getattr| D["Block attribute access"]
E["Command Injection Risks"] -->|Add patterns| F["Enhanced sanitization"]
G["YAML Parser Vulnerabilities"] -->|Segment checking| H["Prevent path traversal"]
B --> I["Security Hardened"]
D --> I
F --> I
H --> I
File Changes1. src/praisonai-agents/praisonaiagents/context/monitor.py
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR tightens security-related validation across the Python CLI tools, the TypeScript YAML workflow loader, and the Python sandbox executor, largely focusing on path traversal/containment and injection hardening.
Changes:
- Replaced string-prefix path containment checks with
Path.relative_to(...)in multiple Python components. - Improved YAML workflow file loading to resolve and consistently use an “effective path”, and refined path traversal detection to check
..path segments. - Hardened the Python sandbox AST validation by expanding the blocked builtin call list.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/praisonai/praisonai/cli/features/agent_tools.py |
Uses Path.relative_to for workspace containment checks; expands command injection pattern detection. |
src/praisonai-ts/src/workflows/yaml-parser.ts |
Moves step-key allowlist to module scope; improves traversal detection; uses resolved effective path for stat/readFile. |
src/praisonai-agents/praisonaiagents/tools/python_tools.py |
Expands blocked builtin calls in the sandbox AST pass (notably adding getattr). |
src/praisonai-agents/praisonaiagents/context/monitor.py |
Uses Path.relative_to to validate monitor output paths remain within the expected directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # SECURITY: Ensure resolved path stays within workspace | ||
| resolved = path.resolve() | ||
| ws_resolved = Path(runtime.config.workspace).resolve() | ||
| if not str(resolved).startswith(str(ws_resolved) + os.sep) and resolved != ws_resolved: | ||
| try: | ||
| resolved.relative_to(ws_resolved) | ||
| except ValueError: | ||
| return json.dumps({"error": f"Directory escapes workspace: {directory}"}) |
There was a problem hiding this comment.
list_files only verifies that the directory resolves within the workspace, but then iterates path.glob() and calls f.stat() on each entry. If a directory contains symlinks to locations outside the workspace, f.stat() will follow the symlink and can leak metadata (and potentially enable later confusion about what is “in” the workspace). Consider resolving each f (or skipping symlinks) and enforcing f.resolve().relative_to(ws_resolved) before including it, and/or using lstat semantics for size collection.
| if isinstance(node, ast.Call) and isinstance(node.func, ast.Name): | ||
| if node.func.id in ('exec', 'eval', 'compile', '__import__', | ||
| 'open', 'input', 'breakpoint', | ||
| 'open', 'input', 'breakpoint', 'getattr', |
There was a problem hiding this comment.
The sandboxed AST check now blocks calls to getattr, but the sandbox also injects a hardened getattr implementation into safe_builtins. This is internally inconsistent (the safe getattr becomes unusable) and makes it harder to reason about the sandbox policy. Either allow getattr calls and rely on _safe_getattr to enforce the restriction, or keep getattr blocked and remove it from safe_builtins (and consider also hardening/blocking hasattr, which can probe dunder names without triggering the ast.Attribute guard).
| 'open', 'input', 'breakpoint', 'getattr', | |
| 'open', 'input', 'breakpoint', |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several security enhancements, including improved path traversal prevention using pathlib.Path.relative_to across multiple modules, a more robust whitelist for YAML workflow step keys, and stricter command sanitization by expanding the list of dangerous patterns. Regarding the security review, the inclusion of getattr in the blocked functions list within python_tools.py is problematic as it conflicts with the existing safe wrapper; it is recommended to remove getattr from the blocklist to allow its safe usage.
| if isinstance(node, ast.Call) and isinstance(node.func, ast.Name): | ||
| if node.func.id in ('exec', 'eval', 'compile', '__import__', | ||
| 'open', 'input', 'breakpoint', | ||
| 'open', 'input', 'breakpoint', 'getattr', |
There was a problem hiding this comment.
The addition of getattr to the blocked functions list in the sandbox AST check creates an inconsistency with _execute_code_direct and effectively disables the safe getattr wrapper provided in safe_builtins (line 200). Since _safe_getattr is specifically designed to prevent access to dunder attributes, getattr should be allowed in the AST check to enable its safe usage.
| 'open', 'input', 'breakpoint', 'getattr', | |
| 'open', 'input', 'breakpoint', |
|
@claude You are working on the PraisonAI SDK. Follow AGENTS.md strictly. STEP 0 — SETUP GIT IDENTITY: STEP 1 — READ GUIDELINES: STEP 2 — ARCHITECTURE VALIDATION & ROUTING (MANDATORY before writing code):
STEP 3 — IMPLEMENT:
STEP 4 — TEST:
STEP 5 — CREATE PR:
|
|
Claude finished @MervinPraison's task —— View job Resolving Merge Conflicts and Security Issues ✅I'll resolve the merge conflicts and address the security issues identified in the reviews. Here's my task list:
✅ Successfully Completed1. Merge Conflicts Resolved
2. Security Issues Addressed
3. Changes Committed and Pushed
The PR now has resolved merge conflicts and addresses the critical security review feedback while maintaining backward compatibility. |
- Resolve merge conflicts with main branch - Fix getattr sandbox inconsistency by removing getattr from blocked AST calls - Combine dangerous patterns from both branches for comprehensive protection - Allow _safe_getattr implementation to work properly in sandbox mode 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <noreply@anthropic.com>
This PR addresses pr #1284
Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor